MB-71397: Fix API that approximates the size of FAISS indexes#75
MB-71397: Fix API that approximates the size of FAISS indexes#75CascadingRadium wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
CascadingRadium
changed the title
faiss_IndexBinary_size and faiss_Index_size
CascadingRadium
requested review from
Likith101,
Nischal1729,
Samsonnyyeet,
Thejas-bhat,
capemox,
Copilot and
maneuvertomars
There was a problem hiding this comment.
Pull request overview
This PR updates the FAISS C “_ex” APIs for index sizing to return a more useful (approximate) in-memory byte estimate instead of effectively returning the pointer size, with the goal of supporting GPU-memory eligibility/throttling decisions when cloning indexes.
Changes:
- Changes
faiss_Index_size/faiss_IndexBinary_sizeto return an error code (int) and write the size to an out-parameter (size_t*). - Implements a new size estimate based on
ntotal * code_sizeplus extra IVF-related overhead (centroids + stored IDs). - Updates/expands header documentation and adds IVF includes to enable IVF-specific handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| c_api/Index_c_ex.h | Updates C API signature + documentation for faiss_Index_size. |
| c_api/Index_c_ex.cpp | Implements the new size estimation logic for float indexes (including IVF path). |
| c_api/IndexBinary_c_ex.h | Updates C API signature + documentation for faiss_IndexBinary_size. |
| c_api/IndexBinary_c_ex.cpp | Implements the new size estimation logic for binary indexes (including IVF path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
looks good but can you please add the MB for it? |
CascadingRadium
changed the title
|
Faiss stores some header information for every index right? I don't believe codes contains that data. We should have additional checks to calculate the header lengths and add it to the size as well |
| return rv; | ||
| int faiss_Index_size(const FaissIndex* index, size_t* p_size) { | ||
| try { | ||
| const faiss::Index* idx = reinterpret_cast<const faiss::Index*>(index); |
There was a problem hiding this comment.
adding to what likith mentioned, i think the header information is accounted partially here with the direct_map accounting on 56, i think having an sizeof(faiss::Index/IndexIVF) would also account for the other field entries. Or is this accounted for as part of "reflectStaticSizefaissIndex" in the go-faiss PR?
There was a problem hiding this comment.
no the "header info" is not accounted for in this PR - this is just a size of the codes. Will add a patch here for the static size. The go-faiss PR is just a go lang size detail, it just captures the size of the pointer (8) to the C struct.
8bytes for thefaiss_IndexBinary_sizeand thefaiss_Index_sizeAPIsquite useless as it always returns
8after performing asizeofoperation.this API cannot be used.
it would require if we bring the entire index to the memory, which would be useful in
approximating how much memory must be available in the GPU to
consider the FAISS index to be eligible for cloning.